Skip to content

feat(5308): Add endpoints to support permission debugging #5408

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 14, 2025
Merged

Conversation

gagantrivedi
Copy link
Member

@gagantrivedi gagantrivedi commented May 1, 2025

Thanks for submitting a PR! Please check the boxes below:

  • I have added information to docs/ if required so people know about the feature!
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have used a Conventional Commit title for this Pull Request

Changes

Implement Endpoints defined here

How did you test this code?

  • tested manually with rbac installed to make sure tags/role stuff is working correctly
  • Adds unit tests

Copy link

vercel bot commented May 1, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
docs ⬜️ Ignored (Inspect) Visit Preview May 14, 2025 11:08am
flagsmith-frontend-preview ⬜️ Ignored (Inspect) Visit Preview May 14, 2025 11:08am
flagsmith-frontend-staging ⬜️ Ignored (Inspect) Visit Preview May 14, 2025 11:08am

@github-actions github-actions bot added api Issue related to the REST API feature New feature or request labels May 1, 2025
Copy link
Contributor

github-actions bot commented May 1, 2025

Uffizzi Preview deployment-63330 was deleted.

@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels May 1, 2025
@gagantrivedi gagantrivedi requested a review from Copilot May 1, 2025 08:06
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements new endpoints and corresponding tests for retrieving detailed permissions information for projects, organisations, and environments to aid in permission debugging. Key changes include:

  • Addition of new detailed permissions endpoints and related test cases.
  • Updates to serializers and permission calculation logic to support the detailed permissions structure.
  • Modifications to views and permission checks to ensure only authorized users can access detailed permission information.

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
api/tests/unit/projects/test_unit_projects_views.py Added tests for detailed permissions endpoint for projects.
api/tests/unit/permissions/test_unit_permissions_calculator.py Added a new test for converting permission data to detailed permissions, along with updates for role permission data.
api/tests/unit/organisations/test_unit_organisations_views.py Added tests for detailed permissions endpoint for organisations.
api/tests/unit/environments/test_unit_environments_views.py Added tests for detailed permissions endpoint for environments.
api/projects/views.py Introduced a new action (detailed_permissions) to get detailed permissions for a user.
api/projects/permissions.py Extended permission check to include the detailed_permissions action.
api/permissions/serializers.py Added detailed permissions serializers to support the new endpoint responses.
api/permissions/rbac_wrapper.py Removed an unused RolePermissionData import.
api/permissions/permissions_calculator.py Updated permission data classes and added a method to convert permission data into detailed permissions data.
api/organisations/views.py Added a detailed_permissions endpoint for organisations.
api/organisations/permissions/permissions.py Updated permission checks for detailed permissions in organisations.
api/environments/views.py Added a detailed_permissions endpoint for environments.
api/environments/permissions/permissions.py Updated permission checks for detailed permissions in environments.

Copy link

codecov bot commented May 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.65%. Comparing base (8dc92c3) to head (05355e8).
Report is 42 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5408      +/-   ##
==========================================
+ Coverage   97.61%   97.65%   +0.03%     
==========================================
  Files        1237     1234       -3     
  Lines       42975    43299     +324     
==========================================
+ Hits        41952    42282     +330     
+ Misses       1023     1017       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels May 7, 2025
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels May 7, 2025
@tiagoapolo tiagoapolo mentioned this pull request May 8, 2025
4 tasks
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels May 11, 2025
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels May 12, 2025
@gagantrivedi gagantrivedi marked this pull request as ready for review May 12, 2025 01:49
@gagantrivedi gagantrivedi requested a review from a team as a code owner May 12, 2025 01:49
@gagantrivedi gagantrivedi removed the request for review from a team May 12, 2025 01:49
@gagantrivedi gagantrivedi requested a review from Zaimwa9 May 12, 2025 01:49
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels May 12, 2025
Copy link
Contributor

github-actions bot commented May 12, 2025

Docker builds report

Image Build Status Security report
ghcr.io/flagsmith/flagsmith-api-test:pr-5408 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-api:pr-5408 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-frontend:pr-5408 Finished ✅ Results
ghcr.io/flagsmith/flagsmith:pr-5408 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-private-cloud:pr-5408 Finished ✅ Results

@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels May 12, 2025
Copy link
Contributor

@Zaimwa9 Zaimwa9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strong feature and improvement overall (including frontend).

Although there is a deduplication issue to solve and some suggestions on simplifying/encapsulating more the logic in to_detailed_permissions_data where most of the complexity lives now

for permission_key in role_permission.permissions:
add_permission(permission_key, None, role_permission.role)

if self.is_organisation_admin or self.user.admin or self.admin_override:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is working well but it's also quite dense and doing a lot of things (admin resolution, formatting, permission origin, deduplication soon).

It might be worth extracting that into a dedicated PermissionAggregator-like class that could:

  • abstract the different pathes
  • encapsulate more the user/role/group logic
  • add_permission being a private method

Its goal would be to construct the graph and have a final build method returning the UserDetailedPermissionsData

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To give some precisions on what I have in mind, it could be used as follow:

     aggregator= PermissionAggregator()

    for group_permission in self.groups:
        for permission_key in group_permissions.permissions:
            aggregator.add_group_permission(permission_key, group_permission.group, is_admin=group_data.admin)
            
            
        ... similar for roles and users
        
        
        return agg.build(is_admin=self.admin)

@@ -70,6 +82,27 @@ class GroupPermissionData(_PermissionDataBase, _GroupPermissionBase):
pass


@dataclass
class PermissionDerivedFromData:
Copy link
Contributor

@Zaimwa9 Zaimwa9 May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way to handle deduplication without doing a linear scan on the arrays before appending could be to internally use dicts here. Have setter methods to add a group/user and build the list with the deduped values once the permission_map is complete wdyt ?

    _group_map: dict[int, GroupData] # Int being the id
    _role_map: dict[int, RoleData]
    
    def build_derived_from(self) -> None:
         self.groups = list(self._group_map.values())
         self.roles = list(self._role_map.values())

Copy link
Member Author

@gagantrivedi gagantrivedi May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good shout! this should be done now

@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels May 14, 2025
@gagantrivedi gagantrivedi requested a review from Zaimwa9 May 14, 2025 11:26
Copy link
Contributor

@Zaimwa9 Zaimwa9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks for changes on to_detailed_permissions_data (and the rbac fix), imo it became really easy to understand the logic behind.

@gagantrivedi gagantrivedi merged commit bdb9bc1 into main May 14, 2025
30 checks passed
@gagantrivedi gagantrivedi deleted the feat/5308 branch May 14, 2025 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue related to the REST API feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants